Fix out of bounds read in bit chunk iterator #416
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jhorstmann
commented
Jun 6, 2021
| let next = | ||
| unsafe { std::ptr::read_unaligned(raw_data.add(index + 1)).to_le() }; | ||
| let next = unsafe { | ||
| std::ptr::read_unaligned(raw_data.add(index + 1) as *const u8) as u64 |
There was a problem hiding this comment.
The fix here is casting the pointer back to *u8 and reading only a single byte.
The other changes are a bit of a cleanup, the masking of next below should not be needed since it masked of exactly the bits that would be shifted out.
There was a problem hiding this comment.
Since this is not the remainder, don't we potentially need to read more than 8 bits? I.e. doesn't this index contain between 1 and 63 bits that need to be "merged" into current?
I get a feeling that this will ignore all bits after the 8th and less than 64. At least this is what I remember from fixing it in arrow2 here.
There was a problem hiding this comment.
The constructor ensures that the bit_offset is between 0..8, this means we need to be able to read unaligned u64, but need at most one additional byte. I'll add this as a comment.
There was a problem hiding this comment.
You are right 👍 Good thinking. Thanks for the clarification.
alamb
approved these changes
Jun 7, 2021
There was a problem hiding this comment.
Thank you @jhorstmann
I reviewed the code and ran test_iter_remainder_out_of_bounds under MIRI on my machine:
cargo +nightly miri test -p arrow -- test_iter_remainder_out_of_bounds
With the code change in this PR:
No failures are reported
Without the code change in this PR:
running 1 test
test util::bit_chunk_iterator::tests::test_iter_remainder_out_of_bounds ... error: Undefined Behavior: memory access failed: pointer must be in-bounds at offset 4103, but is outside bounds of alloc1293907 which has size 4096
--> /Users/alamb/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:788:9
|
788 | copy_nonoverlapping(src as *const u8, tmp.as_mut_ptr() as *mut u8, mem::size_of::<T>());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ memory access failed: pointer must be in-bounds at offset 4103, but is outside bounds of alloc1293907 which has size 4096
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
= note: inside `std::ptr::read_unaligned::<u64>` at /Users/alamb/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:788:9
note: inside `<util::bit_chunk_iterator::BitChunkIterator as std::iter::Iterator>::next` at arrow/src/util/bit_chunk_iterator.rs:144:26
--> arrow/src/util/bit_chunk_iterator.rs:144:26
|
144 | unsafe { std::ptr::read_unaligned(raw_data.add(index + 1)).to_le() };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= note: inside `<util::bit_chunk_iterator::BitChunkIterator as std::iter::Iterator>::fold::<std::option::Option<u64>, fn(std::option::Option<u64>, u64) -> std::option::Option<u64> {std::iter::Iterator::last::some::<u64>}>` at /Users/alamb/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/iter/traits/iterator.rs:2111:29
= note: inside `<util::bit_chunk_iterator::BitChunkIterator as std::iter::Iterator>::last` at /Users/alamb/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/iter/traits/iterator.rs:242:9
note: inside `util::bit_chunk_iterator::tests::test_iter_remainder_out_of_bounds` at arrow/src/util/bit_chunk_iterator.rs:268:30
--> arrow/src/util/bit_chunk_iterator.rs:268:30
|
268 | assert_eq!(u64::MAX, bitchunks.iter().last().unwrap());
| ^^^^^^^^^^^^^^^^^^^^^^^
note: inside closure at arrow/src/util/bit_chunk_iterator.rs:259:5
--> arrow/src/util/bit_chunk_iterator.rs:259:5
|
259 | / fn test_iter_remainder_out_of_bounds() {
260 | | // allocating a full page should trigger a fault when reading out of bounds
261 | | const ALLOC_SIZE: usize = 4 * 1024;
262 | | let input = vec![0xFF_u8; ALLOC_SIZE];
... |
269 | | assert_eq!(0x7F, bitchunks.remainder_bits());
270 | | }
| |_____^
alamb
pushed a commit
that referenced
this pull request
Jun 8, 2021
* Fix out of bounds read in bit chunk iterator * Add comment why reading one additional byte is enough
alamb
added a commit
that referenced
this pull request
Jun 9, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #198.
Rationale for this change
The previous code could read a few bytes out of bounds. I could not cause this to trigger a segmentation fault, but miri also detected a problem and can now run the tests without errors.
What changes are included in this PR?
Are there any user-facing changes?
No